Refactor responsive logic for grid column spans. #59057
Conversation
tellthemachines
added
[Type] Bug
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
This pull request has changed or added PHP files. Please confirm whether these changes need to be synced to WordPress Core, and therefore featured in the next release of WordPress. If so, it is recommended to create a new Trac ticket and submit a pull request to the WordPress Core Github repository soon after this pull request is merged. If you're unsure, you can always ask for help in the #core-editor channel in WordPress Slack. Thank you! ❤️ View changed files❔ lib/block-supports/layout.php |
|
Size Change: +22 B (0%) Total Size: 1.71 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 351b6c8. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7912481153
|
There was a problem hiding this comment.
Testing nicely for me so far! Just found an edge case for blocks with child span set that are then dragged within other types of Group blocks elsewhere on a page (e.g. to within a Row block). I think we might be able to mitigate that if we also check that the parent layout is grid before outputting the rules.
What do you think?
| */ | ||
| if ( columnSpan && parentColumnWidth ) { | ||
| if ( columnSpan && ( minimumColumnWidth || ! columnCount ) ) { |
There was a problem hiding this comment.
Similar to the PHP side of things, do we also need to check that the parent layout type is grid?
There was a problem hiding this comment.
We could actually do this here because the parentLayout comes from the BlockList context and seems to always have the type, but not sure if it's worth having different logic here and in the PHP.
There was a problem hiding this comment.
Yeah, I wouldn't worry about it for now. We can always follow up if need be!
| */ | ||
| if ( isset( $block['attrs']['style']['layout']['columnSpan'] ) && isset( $block['attrs']['style']['layout']['parentColumnWidth'] ) ) { | ||
| if ( isset( $block['attrs']['style']['layout']['columnSpan'] ) && ( isset( $block['parentLayout']['minimumColumnWidth'] ) || ! isset( $block['parentLayout']['columnCount'] ) ) ) { |
There was a problem hiding this comment.
Should we also check that $block['parentLayout']['type'] is set and is grid? This is potentially edge casey, but if I take a block that is a child of a grid layout and then drag it to be within another group (i.e. within a Row block elsewhere on the page), then the block still has columnSpan set, and the parentLayout does not have columnCount (because the parent layout is flex), but this condition still passes, which results in rules being output that likely shouldn't be:
There was a problem hiding this comment.
Good question! So I opted for not doing that because $block['parentLayout']['type'] isn't always set. If grid is defined as the default layout type for a block, and no customisations are made, $block['parentLayout'] will be empty.
I don't think it's a huge issue because the rules we're applying to children (of both flex and grid) are specific to that layout type, so there shouldn't be any weird side-effects.
There was a problem hiding this comment.
Sounds good! Yeah, these rules don't wind up affecting anything visually, so seems fine to leave it for now 👍
|
Ugh I forgot to add the props message 😭 anyone know if it can be added after merge? |
I'd imagine only by updating the commit on |
* trunk: (78 commits) Components: Use `Element.scrollIntoView()` instead of `dom-scroll-into-view` (#59085) DataViews: Correctly display featured image that don't have image sizes (#59111) Elements: Fix block instance element styles for links applying to buttons (#59114) Allow editing of image block alt and title attributes in content only mode (#58998) Add toggle for grid types and stabilise Grid block variation. (#59051) Global Styles: fix console error in block preview (#59112) Navigation: Avoid using embedded records from fallback API (#59076) Refactor responsive logic for grid column spans. (#59057) Editor: Unify Mode Switcher component between post and site editor (#59100) Move StopEditingAsBlocksOnOutsideSelect to Root (#58412) Fix logic error in #58951 (#59101) Block-editor: Auto-register block commands (#59079) Fix small typo in rich text reference guide (#59089) Components: Add lint rules for theme color CSS var usage (#59022) Enter editing mode via Enter or Spacebar (#58795) Updating Storybook to v7.6.15 (latest) (#59074) CustomSelectControl (v1 & v2): Fix errors in unit test setup (#59038) Add stylelint rule to prevent theme CSS vars outside of wp-components (#59020) ColorPicker: Style without accessing InputControl internals (#59069) Pattern block: batch replacing actions (#59075) ...
|
@tellthemachines a git bisect pointed to this PR breaking block instance element styles. I believe it's due to the elements block support hashing the This situation does highlight the fragility of the element block supports but it's a regression all the same. More details can be found over on the issue that reported this. |
|
I also came across this PR when I was researching #59506. As stated in this comment, I came to exactly the same conclusion as @aaronrobertshaw explained 😅 |
|
What a tricky situation with the element styles! Would it be too hacky to explicitly Around this part of the elements support? gutenberg/lib/block-supports/elements.php Line 93 in 01708f1
Agreed. It's unfortunate that the element block support is looking at more data than it needs to generate its classname, while also not allowing any changes to occur to the block instance in this hook 🤔 |
|
Nothing stopping third party plugins from using these filters so definitely need to change the hashing approach. |
| * If it does, add the parent layout to the parsed block. | ||
| */ | ||
| if ( $parent_block && isset( $parent_block->parsed_block['attrs']['layout'] ) ) { | ||
| $parsed_block['parentLayout'] = $parent_block->parsed_block['attrs']['layout']; |
There was a problem hiding this comment.
Editing the $parsed_block in a filter is messing with Interactivity API server side rendering.
Variables comparing function:
https://github.com/WordPress/wordpress-develop/blob/9a616a573434b432a5efd4de21039250658371fe/src/wp-includes/interactivity-api/interactivity-api.php#L49
I'm just writing this so I can follow it up on Monday, I need to work on a way to prevent SSR processing errors when someone filters a parsed_block (Thinking about giving last priority possible, as we compare in render_block filter, after all render_block_data happened. )
There was a problem hiding this comment.
For the record, we are comparing the parsed_block array ourselves because modifying it (adding a key like $parsed_block['is_root_interactive_block'] = true) broke the md5 comparison done in the block supports:
- Related PR: Fix md5 class messed up with new block key #52557
- Block supports use of parsed_block md5: https://github.com/WordPress/wordpress-develop/blob/9a616a573434b432a5efd4de21039250658371fe/src/wp-includes/block-supports/elements.php#L19
There was a problem hiding this comment.
Thanks for the heads up! Yeah the issue with elements block supports already surfaced (see comment above) and was fixed in #59535. render_block_data is a publicly available filter which any extension could use to alter the content of $parsed_block so we can't assume it to be static. The fix will be added to core in 6.6; I guess the Interactivity API logic should also be updated accordingly.
There was a problem hiding this comment.
Awesome! So, we will be able to set something like $parsed_block['is_root_interactive_block'] = true during render_block_data, and the check if it exists during render_block, right?
There was a problem hiding this comment.
Yeah, that should work fine now!
What?
Follow-up on this discussion; thanks to @noisysocks for suggesting the
render_block_dataapproach.Also fixes a bug introduced in #58539 that caused child controls to not display for blocks that have
flexas the default layout (see Navigation block).Testing Instructions
Testing Instructions for Keyboard
Screenshots or screencast